Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V1.1.7 internal audit4.1 #121

Merged
merged 3 commits into from
Oct 6, 2023
Merged

V1.1.7 internal audit4.1 #121

merged 3 commits into from
Oct 6, 2023

Conversation

77ph
Copy link
Collaborator

@77ph 77ph commented Oct 4, 2023

Internal audit based on v.1.1.7 focused on staking contracts.

@77ph 77ph changed the base branch from main to service_staking_tests October 4, 2023 11:51
@77ph 77ph changed the base branch from service_staking_tests to service_staking2 October 4, 2023 11:52
audits/README.md Outdated
@@ -10,6 +10,8 @@ contracts is located in this folder: [internal audit 2](https://github.com/valor
An internal audit with a focus on one-shot upgrade and operating services with signatures
contracts is located in this folder: [internal audit 3](https://github.com/valory-xyz/autonolas-registries/blob/main/audits/internal3).

An internal audit with a focus on "staking"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
An internal audit with a focus on "staking"
An internal audit with a focus on Service Staking

If you wish, you can fight, for example, using this method
hash(multisig.code) vs well-know hash
```
Is it possible to replace the multisig with some kind of fake one without losing the opportunity to receive rewards? <br>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the service has been staked, there is no possibility to update the service multisig.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before staking.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, good point. Added this in registries vulnerabilities list as well!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically for staking to be safe we first need to upgrade the protocol to ensure all services use Safe multisigs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidMinarsch There's no immediate urgency for an upgrade. Specifically, we can perform a direct check on the staking contract when a service is staked under this initial version. Subsequently, we can consider upgrading the GnosisSafeSameAddressMultisig

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the verification in this contract will be enough to close the problem now.
Then we will need to add this check to old contracts.
Everything is as @mariapiamo said. @kupermind has already made such a fix in post-audit PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will do it in the staking contract, however it would be good to update GnosisSafeSameAddressMultisig contract at some point to check for the multisig proxy code.

Base automatically changed from service_staking2 to main October 6, 2023 13:32
@kupermind kupermind merged commit e16be7e into main Oct 6, 2023
2 checks passed
@kupermind kupermind deleted the v1.1.7-internal-audit4.1 branch October 6, 2023 13:32
77ph pushed a commit that referenced this pull request Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants